-
Notifications
You must be signed in to change notification settings - Fork 799
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Infinite scroll added checks to validate input #39618
base: trunk
Are you sure you want to change the base?
Infinite scroll added checks to validate input #39618
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. 🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so:
Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
@@ -779,7 +780,7 @@ public function query_time_filter( $where, $query ) { | |||
* @param string $last_post_date @deprecated Last Post Date timestamp. | |||
*/ | |||
$operator = 'ASC' === $_REQUEST['query_args']['order'] ? '>' : '<'; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotValidated -- no changes to the site. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this assign makes sense anymore since in line 756 we return early if 'DESC' !== $_REQUEST['query_args']['order']
@jeherve I see you have been working with Infinite Scroll in the past. Is line 756 a bug or should we directly do $operator = '<'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the $operator and $last_post_date were supposed to be deprecated.
jetpack/projects/plugins/jetpack/modules/infinite-scroll/infinity.php
Lines 779 to 780 in d9973a1
* @param string $operator @deprecated Query operator. | |
* @param string $last_post_date @deprecated Last Post Date timestamp. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't know that this parameter is needed at all anymore. It seems like it would be safe to remove, maybe using apply_filters_deprecated()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I don't know that this parameter is needed at all anymore. It seems like it would be safe to remove, maybe using
apply_filters_deprecated()
TBH I don't even like the filter since it is supposed to be something similar to posts_where but it works differently since it adds to the $where
and doesn't replace it the way posts_where
does.
By checking the official documentation, it looks like we would need to replace it with a new filter ($replacement
), but the $args
are the same ones for both new and replacement filters which defeats the purpose of removing $operator
.
Would something like the below diff work? I have not added a replacement purposefully since the idea is to remove the filter and the fields entirely in a future release.
diff --git a/projects/plugins/jetpack/modules/infinite-scroll/infinity.php b/projects/plugins/jetpack/modules/infinite-scroll/infinity.php
index 52929d303b..f6a2c3f5b0 100644
--- a/projects/plugins/jetpack/modules/infinite-scroll/infinity.php
+++ b/projects/plugins/jetpack/modules/infinite-scroll/infinity.php
@@ -778,9 +778,9 @@ class The_Neverending_Home_Page {
* @param string $operator @deprecated Query operator.
* @param string $last_post_date @deprecated Last Post Date timestamp.
*/
- $operator = 'ASC' === $_REQUEST['query_args']['order'] ? '>' : '<'; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotValidated -- no changes to the site.
- $last_post_date = sanitize_text_field( wp_unslash( $_REQUEST['last_post_date'] ) ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotValidated -- no changes to the site.
- $where .= apply_filters( 'infinite_scroll_posts_where', $clause, $query, $operator, $last_post_date );
+ $operator = '<';
+ $last_post_date = isset( $_REQUEST['last_post_date'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['last_post_date'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- no changes to the site
+ $where .= apply_filters_deprecated( 'infinite_scroll_posts_where', array( $clause, $query, $operator, $last_post_date ), '$$next-version$$', '' );
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that would be okay. 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in 3b49148
Updated the test instructions too
Fixes Warnings in Infinite scroll Module like:
Undefined array key "query_before"
Undefined array key "query_args"
Undefined array key "last_post_date"
118d8bdb2d4600db50a5c7b306828911-logstash
Proposed changes:
infinite_scroll_posts_where
hookTesting instructions:
Code Review
Edited: For the changes when deprecating the
infinite_scroll_posts_where
.